Server implementation of Streamable HTTP transport #266
Conversation
src/server/streamableHttp.ts
Outdated
| /** | ||
| * The session ID SHOULD be globally unique and cryptographically secure (e.g., a securely generated UUID, a JWT, or a cryptographic hash) | ||
| * | ||
| * When sessionId is not set, the transport will be in stateless mode. | ||
| */ | ||
| sessionId: string | undefined; |
There was a problem hiding this comment.
Oops—I know I suggested this, but I think we actually need to do something different. It'd probably be better for this to accept a function (or undefined) which generates a session ID.
Then, the transport can call the function if it receives an InitializeRequest, but won't call it to generate a session ID otherwise. This seems valuable to avoid wasteful computation, but also to more clearly indicate the semantics here.
| // Mock IncomingMessage | ||
| function createMockRequest(options: { | ||
| method: string; | ||
| headers: Record<string, string | string[] | undefined>; | ||
| body?: string; | ||
| }): IncomingMessage { | ||
| const readable = new Readable(); | ||
| readable._read = () => { }; | ||
| if (options.body) { | ||
| readable.push(options.body); | ||
| readable.push(null); | ||
| } | ||
|
|
||
| return Object.assign(readable, { | ||
| method: options.method, | ||
| headers: options.headers, | ||
| }) as IncomingMessage; | ||
| } | ||
|
|
||
| // Mock ServerResponse | ||
| function createMockResponse(): jest.Mocked<ServerResponse> { | ||
| const response = { | ||
| writeHead: jest.fn().mockReturnThis(), | ||
| write: jest.fn().mockReturnThis(), | ||
| end: jest.fn().mockReturnThis(), | ||
| on: jest.fn().mockReturnThis(), | ||
| emit: jest.fn().mockReturnThis(), | ||
| getHeader: jest.fn(), | ||
| setHeader: jest.fn(), | ||
| } as unknown as jest.Mocked<ServerResponse>; | ||
| return response; | ||
| } |
There was a problem hiding this comment.
Claude says the tests are good, so I'll defer to Claude 😂
However… we may want to consider not mocking these classes. Feels fragile and error-prone. It might be better to actually just spin up an HTTP server in the tests. Anyway, this isn't blocking—can do it in a follow-up if we want.
There was a problem hiding this comment.
I'll do in the follow up if it's okay, planning to add more comprehensive tests with client implementation and the complete e2e flow
|
Looks good, just want to check regarding the non-SSE case (batched JSON response) is not supported or mentioned in the follow-ups - would it be implemented in this class later or as a separate transport? |
@beaulac good point, yes, it's planned, will add it to the follow-ups |
github-project-automation
bot
moved this from In Progress
to Done
in 2025-03-26 Implementation
| })); | ||
| return; | ||
| } | ||
| this.sessionId = this.sessionIdGenerator(); |
There was a problem hiding this comment.
This will throw if this.sessionIdGenerator is undefined - should it default to a () => undefined stub if it isn't passed in the constructor args?
(or could just change to this.sessionId = this.sessionIdGenerator?.();)
|
Does this also contribute the Streamable HTTP client, or is it server only? |
|
Hi @ihrpr, in protocol.ts
|
|
It appears this wasn't included in the latest release. Is there a release schedule I can look to so I can make plans around things? Also, it doesn't look like any doc updates were included in this pull request. |
@kentcdodds I'll update docs once the the entire transport implementation is ready, including client and examples how to use and a few follow ups. |
|
Ah, I missed those PRs. Thanks @ihrpr! |
| const hasRequests = messages.some(msg => 'method' in msg && 'id' in msg); | ||
| const hasOnlyNotificationsOrResponses = messages.every(msg => | ||
| ('method' in msg && !('id' in msg)) || ('result' in msg || 'error' in msg)); |
There was a problem hiding this comment.
Are these ever both going to be false? (Maybe all errors?) If so we will fail to return a response at all unless I'm missing something.
If they are never both going to be false do you need both?
There was a problem hiding this comment.
If they are never both going to be false do you need both?
Yes agreed, hasOnlyNotificationsOrResponses could probably be inlined into !hasRequests.
There should be different validation on the messages based on the spec since
The body of the POST request MUST be one of the following:
A single JSON-RPC request, notification, or response
An array batching one or more requests and/or notifications
An array batching one or more responses
is not correctly enforced as-is.
If so we will fail to return a response at all
Yes - this is to satisfy:
If the server accepts the input, the server MUST return HTTP status code 202 Accepted with no body.
(should actually probably be reworded to mention "accepts the input when there are no requests" or something, CTTOI)
and accords with JSON-RPC batch spec
A Response object SHOULD exist for each Request object
...
there SHOULD NOT be any Response objects for notifications
...
If there are no Response objects contained within the Response array as it is to be sent to the client, the server MUST NOT return an empty Array and should return nothing at all.
so res.writeHead(202).end(); below intentionally responds immediately with no body, the server can process the messages without blocking the ServerResponse.
This should be clarified with some additional code comments, imo.
Enhance Sidebar component accessibility
Introduces support for the new Streamable HTTP transport on the server side.
Builds on the work of @gylove1994 in #230.
To do
Follow ups